CPP: Work on Lossy function result cast query#654
Conversation
jbj
left a comment
jbj
left a comment
There was a problem hiding this comment.
I have mixed feelings about this query. It's more or less a compiler warning, and we generally leave that type of query to compilers. It would make more sense if we could reduce FPs by taking a whole-program view.
On the other hand, I ran this query on a bunch of projects, and the results generally looked good. Unexpected rounding probably won't be a disaster for these projects, but almost all of them could use an explicit cast or rounding to at least show the reader that it's not an accident.
| fName = "roundl" or | ||
| fName = "trunc" or | ||
| fName = "truncf" or | ||
| fName = "truncl" |
There was a problem hiding this comment.
My main worry is that this list is that this list isn't exhaustive. It doesn't include user-defined wrappers around rounding functions, for example. I also notice that pow(2, smallPositiveInteger) is a common FP.
There was a problem hiding this comment.
I didn't find any cases of user-defined wrappers 'in the wild', but I've added this functionality.
I've also add pow (and variants) as an exception, provided the first number (base) has a compile time value that is a whole number.
There was a problem hiding this comment.
I ran the query again on a bunch of LGTM projects and found this FP, where __builtin_ceil is called. Apparently, GCC has builtin versions of some of these operations. AFAICT from the compiler docs, only the ceil and floor functions are available as builtins, but the mathop.h file also uses __builtin_lroundf and apparently gets away with it. Perhaps, to be on the safe side, we should just whitelist all these function names with a __builtin_ prefix as well.
There was a problem hiding this comment.
Thanks for investigating. I've added an exception for __builtin_* as well.
geoffw0
force-pushed
the
lossyresultcast
branch
from
c5e96e8 to
7a877bf
Compare
That's a fair criticism.
That was my observation as well, hence my instinct to make it run but not displayed by default on LGTM. I'll remove the |
|
Testing here: https://lgtm.com/query/351600969242589109/ |
| fName = "roundl" or | ||
| fName = "trunc" or | ||
| fName = "truncf" or | ||
| fName = "truncl" |
There was a problem hiding this comment.
I ran the query again on a bunch of LGTM projects and found this FP, where __builtin_ceil is called. Apparently, GCC has builtin versions of some of these operations. AFAICT from the compiler docs, only the ceil and floor functions are available as builtins, but the mathop.h file also uses __builtin_lroundf and apparently gets away with it. Perhaps, to be on the safe side, we should just whitelist all these function names with a __builtin_ prefix as well.
| rs.getEnclosingFunction() = fc.getTarget() and | ||
| rs.getExpr().(VariableAccess).getTarget() = v and | ||
| whiteListWrapped(v.getAnAssignedValue()) | ||
| ) |
There was a problem hiding this comment.
The wrapper detection is failing to detect vim_round, and this causes a FP on vim. I suggest using DataFlow::localFlow.
There was a problem hiding this comment.
Good thinking, done.
jbj
left a comment
jbj
left a comment
There was a problem hiding this comment.
Tested at https://lgtm.com/query/7450163572923171309/. The tests hit a transient failure, and I've re-triggered them.
3 participants
Added test, qhelp and tags to this query. Fixed some false positives. It appears to produce generally good results (despite the comment that was in the
@description), so I've given it amediumprecision - which will result on it being run on LGTM but not displayed by default. If this part turns out to be controversial I can reduce the precision tolowor delete it.@jbj - I'd like your opinion on the precision tag.
@Semmle/doc - please review and make suggestions on the new
qhelp.